[release/10.0] [mono][interp] Add missing intrinsic for Volatile.ReadBarrier/WriteBarrier - #130303
[release/10.0] [mono][interp] Add missing intrinsic for Volatile.ReadBarrier/WriteBarrier#130303github-actions[bot] wants to merge 2 commits into
Conversation
…rrier For simplicity, this just adds full memory barriers. Previously, calling this code would lead to stack overflow due to recursively calling itself.
|
Tagging subscribers to this area: @vitek-karas, @BrzVlad, @kotlarmilos |
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com> Co-authored-by: Jan Kotas <jkotas@microsoft.com> Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
BrzVlad
added
Servicing-approved
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "765b8f1dd0d0e6e74d4fdf6adaaf497e46a18f36",
"last_dispatched_base_ref": "release/10.0",
"last_dispatched_base_sha": "aced99138b1efad4a5361ff68ea1951c860dcbda",
"last_reviewed_commit": "765b8f1dd0d0e6e74d4fdf6adaaf497e46a18f36",
"last_reviewed_base_ref": "release/10.0",
"last_reviewed_base_sha": "aced99138b1efad4a5361ff68ea1951c860dcbda",
"last_recorded_worker_run_id": "29681939970",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "765b8f1dd0d0e6e74d4fdf6adaaf497e46a18f36",
"review_id": 4730543769
}
]
} |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Holistic Review
Motivation: This is a backport of #124538 to release/10.0. The Volatile.ReadBarrier/Volatile.WriteBarrier APIs are new in .NET 10. Under the Mono interpreter these methods had no intrinsic handling, so calls crash. This impacts iOS, Android debug, and WASM scenarios, and a customer hit it after upgrading Microsoft.CodeAnalysis.CSharp.
Approach: Adds an intrinsic case in interp_handle_intrinsics that maps parameterless System.Threading.Volatile.ReadBarrier/WriteBarrier to MINT_MONO_MEMORY_BARRIER, mirroring the existing Thread.MemoryBarrier handling. A new VolatileTests set exercises the barriers directly, via reflection, and interleaved with Volatile.Read/Write, and is wired into the test csproj.
Summary: The change is minimal, self-contained, and faithfully matches the original PR. Emitting a full memory barrier is conservatively correct for both the acquire-style read barrier and the release-style write barrier, and the param_count == 0 guard keeps the intrinsic scoped to the intended overloads. The test asserts only that the calls do not throw (the equality assertions on single-threaded, non-shared locals are trivially true), which aligns with the crash-fix intent. No correctness, security, or performance concerns. Verdict: LGTM.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 40.7 AIC · ⌖ 10.2 AIC · ⊞ 10K
Backport of #124538 to release/10.0
/cc @BrzVlad
Customer Impact
Applications calling
Volatile.ReadBarrier/WriteBarrierwill crash when running under mono interpreter. This can impact ios, android debug and wasm. A customer upgrading to Microsoft.CodeAnalysis.CSharp 5.6.0 encountered this error.Regression
This new api was added in .NET10. As more libraries will use this new API, this problem will become more frequent.
Testing
Tested on simple app calling this API.
Risk
Low. This just adds an intrisic for this API which conservatively emits a full memory barrier.